-
-
Notifications
You must be signed in to change notification settings - Fork 355
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add tabs and container components #1549
Conversation
Thanks for this effort, seems to work well! I'm not sure if I like the approach with the
I think this alternative sounds a lot better and more intuitive. Then in the This might require special logic in the render overlay or the page view state so that when we dropped an element in an |
At the moment yes, there is nothing else that could use this prop, however I think controlling visibility is something that we currently lack in general and it should be useful. I.e. if we built a form with checkbox/radio and expected that some content would appear depending on selection, this component could also be used. Unless we decide on some different method to control visibility of components |
Yeah that's a good point. It sounds fine to me to keep the prop in the |
|
||
export default createComponent(Container, { | ||
argTypes: { | ||
children: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thing: there is a bug I've found so if this prop is not called children
it doesn't work, but it should. I'm including the fix in #1527
Interesting! I think for now this looks good to me. |
@prakhargupta1 yes ofc, I can add it. I just wanted to share this option quickly, but if we are fine with intermediate solution then I can improve it as much as needed and work on tabbed container improvements in follow up |
We will need to expand on the export interface ObjectValueType extends ValueTypeBase {
type: 'object';
/** @deprecated lets remove this: */
schema?: string;
properties?: Record<string, PropValueType>
}
export interface ArrayValueType extends ValueTypeBase {
type: 'array';
/** @deprecated lets remove this: */
schema?: string;
items?: PropValueType
} that way we can define something like: argTypes: {
tabs: {
typedef: {
type: 'array',
items: {
type: 'object',
properties: {
label: { type: string },
content: { type: 'element' }
}
}
}
}
} Not 100% sure this will be enough to solve the tabs problem. To be further investigated. I think this should happen as a separate effort from adding new components |
@apedroferreira added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me as a first approach!
Like I said maybe we could consider also controlling visibility with the sx
prop instead of having a prop just for it in the Containers, but I'll leave that up to you, as we can also improve this with the more difficult solution later.
export default createComponent(Tabs, { | ||
layoutDirection: 'horizontal', | ||
argTypes: { | ||
active: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify what you mean? the name of prop? if yes, why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the name of prop? if yes, why?
yes, because:
- Conventions: we generally use the combination
value
/onChange
to denote the main controlled prop of a component (yes, there are specific situations where we deviate from this, e.g. DataGrid.) - The MUI Tabs component uses
value
- This component as it currently is, is in essence a Select, and the MUI Select uses
value
- The word "active" is an adjective, and "a tab" is "a thing", so we should use a noun for naming it in our code.
I'm not sure about the tabs. It feels like a One thing to note is that the container allows users creating flows that cause vertical layout shift, which is something we have avoided up until now. |
retool has both
Avoided intentionally? Or is this just part of a change with new features? Also if ensuring no layout shifts happens is so important, users can use containers with fixed height via |
Retool ended up with a lot of components that kinda do the same, but different. And I'm not sure that it's such a good thing. Why would you need But now that I think of it, we can always migrate the current
It was avoided intentionally. The main idea was that by default, when it comes to building good UX, users should fall in a pit of success. Making it hard to build layout shifting UI was part of that. But users were always able to circumvent with the |
CleanShot.2023-01-12.at.14.37.45.mp4
@prakhargupta1 I know it's not a "tabbed container" that was discussed originally in the task, but IMO this comes closer to what we want to achieve right now.
If there won't be objections I would propose to continue polishing the approach as follow up as I realised to do something as a tabbed container we would probably need to do major work on how things are stored in our data structure.
This PR introduces 2 components:
By using Tabs UI we can control visibility of container.
Now to actually do a "tabbed container" which would automatically swap items, I'll probably need some guidance either from @apedroferreira or from @Janpot. If any of you guys have any tips before going that road - how should we handle adding components to a container and switching active container within same component? (asking it here, as I'm not sure if we will agree on a current implementation as a intermediate solution or should that be replaced with more advanced implementation)